Skip to content

Conversation

@vineetbansal
Copy link
Contributor

Summary of Changes

>> Provide context and a description of your changes here. Make sure to reference any associated issues. <<

Requirements

Note: If you are an external contributor, you will see a comment from @buildbot-princeton. This is solely for the maintainers.

@vineetbansal
Copy link
Contributor Author

vineetbansal commented Nov 14, 2025

TODOs and points to discuss:

  • The tests in tests/jobflow are not yet comprehensive. Ideally, all 4 combos of JOBFLOW_AUTO_SUBMIT/JOBFLOW_RESOLVE_FLOW_RESULTS would need to get tested using with change_settings.

  • This is building off a custom branch vb/flow_decorator_quacc in the fork for jobflow that differs from the one (vb/flow_decorator) in the PR in one crucial way - in the official PR the @flow decorator insists that a Job/Flow or OutputReference be returned, to keep it in line with how a Flow operates (i.e. something like flow = Flow([job1, job2], output=[job1.output, job2.output]) would have no effect on the output of the Flow, and the returned dict is still a dict of results from all the jobs in the jobs parameter). See the difference between the 2 here.

This off-PR branch exists only because tests in tests/jobflow/test_syntax.py seems to expect this to work:

  @subflow
  def add_distributed(vals, c):
      return [add(val, c) for val in vals]

This seems to be the way we use flows anyway:

    for slab in slabs:
      result = relax_job(slab)

      if static_job is not None:
          result = static_job(result["atoms"])

      results.append(result)

So perhaps we push these changes upstream or pull them downstream in quacc?

  • Another test in test_syntax.py seems to expect this to work:
    @flow
    def workflow(a, b, c):
        return mult(add(a, b), c)
    ...
    assert isinstance(workflow(1, 2, 3), jf.Flow)

So combining a Job output and a primitive in an operation. This works in Prefect but will not work in our Jobflow flow decorator without sophisticated handling. See comment by gpeteretto - "..I would not see any reasonable way of implementing this, especially for jobflow-remote or fireworks executions. ". So before we tackle this, I wanted to make sure that this is not just a test case but an actual usage pattern in quacc.

  • Do we even need a JOBFLOW_AUTO_SUBMIT? I see that in tests/jobflow/test_tutorials.py we have:
    responses = jf.run_locally(flow, ensure_success=True)
   assert responses[job2.uuid][1].output == 9

So I need to confirm if job submission is the eventual responsibility of user code. If we do support it, then most certainly I'm doing it wrong here:

        if settings.JOBFLOW_AUTO_SUBMIT:
            return run_locally(bound_jobflow_flow, ensure_success=True)

and I need to use jfremote to do this (once corresponding changes are incorporated in jfremote).

  • Start adding documentation!

@vineetbansal
Copy link
Contributor Author

@Andrew-S-Rosen - I'm getting the same error as in the CI locally:

ModuleNotFoundError: No module named 'matgl.apps._pes_pyg'

I can try to fix this, unless you have some ideas already..

@Andrew-S-Rosen
Copy link
Member

The issues are actually slightly different I think. The issue you are getting seems to be related to the fact that the matgl dependency was updated yesterday with breaking changes, and dependabot had not yet bumped the version for me to flag and correct.

The issue in CI right now which is using the prior version of matgl is complaining about some model caching issue, probably also related to changes to the ML models in matgl that were updated on Hugging Face recently. Neither are related to quacc directly. We can chat soon about it.

Removed caching for pip packages in workflow.
@codecov
Copy link

codecov bot commented Nov 14, 2025

Codecov Report

❌ Patch coverage is 94.87179% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 97.61%. Comparing base (a7befad) to head (09536ca).
⚠️ Report is 26 commits behind head on main.

Files with missing lines Patch % Lines
src/quacc/wflow_tools/customizers.py 85.71% 1 Missing ⚠️
src/quacc/wflow_tools/decorators.py 96.87% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3016      +/-   ##
==========================================
- Coverage   98.19%   97.61%   -0.58%     
==========================================
  Files          92       95       +3     
  Lines        3868     4107     +239     
==========================================
+ Hits         3798     4009     +211     
- Misses         70       98      +28     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants